Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @anurags92 and @pawanrawal)

query/shortest.go, line 337 at r1 (raw file):

			newRoute := item.path
			newRoute.totalWeight = item.cost
			kroutes = append(kroutes, newRoute)

if len(newRoute) <= depth ? <- To be discussed

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @anurags92 and @pawanrawal)

query/shortest.go, line 73 at r2 (raw file):

	sort.Slice(tmp, func(i, j int) bool {
		return tmp[i].uid < tmp[j].uid
	})

Slice sorting and the searching is expensive here. Just a linear search would work fine.

query/shortest.go, line 383 at r2 (raw file):

		default:
			if len(kroutes) == numPaths {
				break

This will break out of switch case and not for loop. Use a labelled for loop here.

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @anurags92)

query/query3_test.go, line 683 at r3 (raw file):

	query := `
	{
		A as shortest(from: 51, to:55, numpaths: 5) {

Seems like the output of this one and the test below is the same. Could you give this query various inputs like numpaths 5, 6, 10, 100 etc. and see it still returns the same result.

Could you have a test which checks this for various values of depth as well along with a fixed value of numpaths say 5. Have cases for depth 0, 1, 2, 3, 4, 5, 10 and 100.

query/shortest.go, line 378 at r3 (raw file):

			return nil, ctx.Err()
		default:
			if len(kroutes) == numPaths {

Is this useful here? We already check this when we append to kroutes above. Does anything fail if you remove this?

query/shortest.go, line 389 at r3 (raw file):

				continue
			}
			if len(*item.path.route) > 0 && item.path.indexOf(toUid) != -1 {

This needs a comment on what it does and why is it needed. Also mention about the associated test case here.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @anurags92)

query/shortest.go, line 66 at r3 (raw file):

func (r *route) indexOf(uid uint64) int {

You can pass a pointer to route that could avoid unnecessary copy.

Reviewable status: 0 of 3 files reviewed, 8 unresolved discussions (waiting on @ashish-goswami, @gja, and @pawanrawal)

query/query3_test.go, line 683 at r3 (raw file):

Addressed. Also added the tests which were earlier commented out. They work fine as well.

query/query3_test.go, line 593 at r4 (raw file):

Yes, thanks. I have manually tried to minimize vertical spaces. But except for the name key, other params are nested. So could achieve only partial benefits.

query/shortest.go, line 337 at r1 (raw file):

As discussed in the discuss post, continuing with the current implementation of depth.

query/shortest.go, line 73 at r2 (raw file):

Done.

query/shortest.go, line 383 at r2 (raw file):

no applicable anymore.

query/shortest.go, line 66 at r3 (raw file):

Done.

query/shortest.go, line 378 at r3 (raw file):

Removed.

query/shortest.go, line 389 at r3 (raw file):

// Skip neighbour if the cost is greater than the maximum weight allowed.

query/shortest.go, line 389 at r3 (raw file):

Added. But didn't mention the test cases. It is being checked in multiple ways.

Reviewed 3 of 3 files at r5.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @anurags92, @ashish-goswami, and @gja)

query/query3_test.go, line 1240 at r5 (raw file):

			emptyPath,
		},
		//The test cases below are for k-shortest path queries with varying depths. They don't pass

This comment can be removed

Just ensure that the documentation explains this "changed" behavior.

Reviewable status: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @anurags92, @ashish-goswami, @gja, and @pawanrawal)

Do mention that this is a breaking fix -- if the user's results might change now.

Reviewable status: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @anurags92, @ashish-goswami, @gja, and @pawanrawal)